fix(rate-limiting): counters accuracy with redis policy & sync_rate #11859
+57
−47
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
samugi
force-pushed
the
fix/rate-limiting-redis-sync
branch
2 times, most recently
from
b61685d to
36b8701
Compare
When the periodic sync to redis feature is turned on, using the `sync_rate` configuration option, keys are incremented by steps of 2 instead of 1 for requests that arrive after the `sync_rate` interval has expired. This happens because after each sync, the key is loaded again from redis and also incremented atomically (see: #10559) however the next call to `increment` also adds 1 to its value, so the key is incremented by 2 every time it's loaded from redis. This fix sets a negative delta for the key when `conf.sync_rate ~= SYNC_RATE_REALTIME` and the key was loaded from redis in order to invalidate the next call to `increment`.
samugi
force-pushed
the
fix/rate-limiting-redis-sync
branch
from
36b8701 to
9804330
Compare
samugi
changed the title
ADD-SP
approved these changes
Oct 31, 2023
|
the PR is ready. Holding on merge for a while, since the same logic is being investigated in https://github.com/Kong/kong-ee/pull/6253 just in case it requires any adjustments. @ADD-SP @StarlightIbuki let me know when you think this can be moved forward. (adding do not merge label because of this). Update: no conflicts came up, I think this is ready to merge. |
StarlightIbuki
approved these changes
Nov 7, 2023
windmgc
pushed a commit
that referenced
this pull request
Jan 24, 2024
…11859) * fix(rate-limiting): redis async updates When the periodic sync to redis feature is turned on, using the `sync_rate` configuration option, keys are incremented by steps of 2 instead of 1 for requests that arrive after the `sync_rate` interval has expired. This happens because after each sync, the key is loaded again from redis and also incremented atomically (see: #10559) however the next call to `increment` also adds 1 to its value, so the key is incremented by 2 every time it's loaded from redis. This fix sets a negative delta for the key when `conf.sync_rate ~= SYNC_RATE_REALTIME` and the key was loaded from redis in order to invalidate the next call to `increment`. Includes a small code refactor
3 tasks
windmgc
pushed a commit
that referenced
this pull request
Mar 7, 2024
…11859) * fix(rate-limiting): redis async updates When the periodic sync to redis feature is turned on, using the `sync_rate` configuration option, keys are incremented by steps of 2 instead of 1 for requests that arrive after the `sync_rate` interval has expired. This happens because after each sync, the key is loaded again from redis and also incremented atomically (see: #10559) however the next call to `increment` also adds 1 to its value, so the key is incremented by 2 every time it's loaded from redis. This fix sets a negative delta for the key when `conf.sync_rate ~= SYNC_RATE_REALTIME` and the key was loaded from redis in order to invalidate the next call to `increment`. Includes a small code refactor
windmgc
pushed a commit
that referenced
this pull request
Mar 8, 2024
…11859) * fix(rate-limiting): redis async updates When the periodic sync to redis feature is turned on, using the `sync_rate` configuration option, keys are incremented by steps of 2 instead of 1 for requests that arrive after the `sync_rate` interval has expired. This happens because after each sync, the key is loaded again from redis and also incremented atomically (see: #10559) however the next call to `increment` also adds 1 to its value, so the key is incremented by 2 every time it's loaded from redis. This fix sets a negative delta for the key when `conf.sync_rate ~= SYNC_RATE_REALTIME` and the key was loaded from redis in order to invalidate the next call to `increment`. Includes a small code refactor
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
When the periodic sync to redis feature is turned on, using the
sync_rateconfiguration option, keys are incremented by 2 instead of 1 for requests that arrive after thesync_rateinterval has expired.This happens because after each sync, the key is loaded from redis using
incr(see: #10559) however the next call toincrementalso adds 1 to the counter, so the key is incremented twice every time it's loaded from redis. In other words there can be 2 increments for the same request: here and then here.This fix sets a negative delta for the key when
conf.sync_rate ~= SYNC_RATE_REALTIMEand the key was loaded from redis to account for the prior increment.Checklist
changelog/unreleased/kongorskip-changeloglabel added on PR if changelog is unnecessary. README.mdFull changelog
Issue reference
KAG-2906